Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Save properties after applying changes in Dashboard #17570

Merged
merged 26 commits into from
Dec 9, 2021
Merged

fix: Save properties after applying changes in Dashboard #17570

merged 26 commits into from
Dec 9, 2021

Conversation

geido
Copy link
Member

@geido geido commented Nov 29, 2021

SUMMARY

This PR fixes a severe issue for which changes in the properties modal of a Dashboard won't be fully saved when the onlyApply prop was used. This PR #17392 enabled the onlyApply tag. Later, manual tests have shown that the tag wasn't ready to be used as both the frontend and the backend were not fully implemented for that to work properly. In addition to that, this PR does the following:

  • Refactors the PropertiesModal to use typescript
  • Refactors the form component to use the functionalities provided by Antdesign
  • Fixes an issue in the Dashboard detail page for which when applying changes and re-opening the properties modal it did not show the latest changes
  • Fixes an issue for which changes in the json_metadata for color_namespace, expanded_slices, timed_refresh_immune_slices were not saved
  • Fixes an issue for which the dashboard wasn't redirecting back to the original URL when the slug was deleted
  • Fixes an issue for which changes to native filters would not update the json_metadata immediately, causing potential overwrites
  • Changes the endpoint that was used for updating the dashboard (save_dash) with the standard PUT endpoint. Also, it adds the set_dash_metadata function to the update command in the backend. The set_dash_metadata function was only used by the save_dash endpoint before, causing potential discrepancies in the way the json_metatada was handled by the standard PUT endpoint.
  • Removes unnecessary fetch requests

TESTING INSTRUCTIONS

  1. Open a Dashboard
  2. Edit the properties, including the json metadata
  3. Apply the changes
  4. Reopen the modal and make sure the latest changes are there
  5. Finally, save the changes
  6. Reload the Dashboard and make sure the properties were fully saved

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@geido geido marked this pull request as ready for review December 1, 2021 19:49
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #17570 (c79aa5f) into master (8c25f2f) will decrease coverage by 0.03%.
The diff coverage is 63.63%.

❗ Current head c79aa5f differs from pull request most recent head 9bb937e. Consider uploading reports for the commit 9bb937e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17570      +/-   ##
==========================================
- Coverage   68.86%   68.82%   -0.04%     
==========================================
  Files        1598     1598              
  Lines       65297    65370      +73     
  Branches     6952     6961       +9     
==========================================
+ Hits        44966    44993      +27     
- Misses      18446    18486      +40     
- Partials     1885     1891       +6     
Flag Coverage Δ
javascript 57.42% <57.84%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...et-frontend/src/dashboard/components/SaveModal.tsx 51.16% <0.00%> (+1.16%) ⬆️
...uperset-frontend/src/utils/getClientErrorObject.ts 71.87% <ø> (ø)
...t-frontend/src/dashboard/actions/dashboardState.js 35.79% <36.84%> (+0.45%) ⬆️
...frontend/src/dashboard/components/Header/index.jsx 61.11% <37.50%> (+2.08%) ⬆️
superset/dashboards/api.py 91.86% <57.14%> (ø)
superset/dashboards/commands/update.py 83.07% <66.66%> (ø)
...src/dashboard/components/PropertiesModal/index.tsx 66.87% <66.87%> (ø)
superset/dashboards/dao.py 96.09% <97.36%> (ø)
superset/dashboards/schemas.py 99.40% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c25f2f...9bb937e. Read the comment docs.

Comment on lines -100 to -103
cy.wait('@dashboardGet').then(() => {
selectColorScheme('d3Category20b');
});

Copy link
Member Author

@geido geido Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was here because the color scheme was considered to be mandatory. This is not the case any longer

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I left a few cosmetic comments.

Since this PR is very large, it would be great if more people took a look at the code

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! LGTM, but like I said, it would be great to have 1 more review since it's a big PR

@rusackas
Copy link
Member

rusackas commented Dec 8, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

@rusackas Ephemeral environment spinning up at http://34.217.207.248:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@eschutho eschutho added the v1.4 label Dec 8, 2021
}

Modal.error({
title: 'Error',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a t() here - not worth holding up the PR over this though. Looks like there are a few more of these floating around in the codebase to sweep up anyway

content: t('A valid color scheme is required'),
okButtonProps: { danger: true, className: 'btn-danger' },
});
throw new Error('A valid color scheme is required');
Copy link
Member

@rusackas rusackas Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error('A valid color scheme is required');
throw new Error(t('A valid color scheme is required'));

Could use a t() here. Not blocking the PR for this!

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for all this hard work!

@rusackas rusackas merged commit 12bd1fc into apache:master Dec 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

Ephemeral environment shutdown and build artifacts deleted.

@suddjian
Copy link
Member

Thanks for this fix! Embedded work has been keeping me busy so thanks to the others for picking up the review. I managed to finally take a bit of a look today and it LGTM too. The original code here was one of my first Superset contributions 😁

eschutho pushed a commit that referenced this pull request Dec 11, 2021
* Refactor PropertiesModal

* Update json_metadata fully

* Clean up

* Verify values

* Catch changed to metadata

* Always updated dashboard info on update

* Avoid unnecessary fetches

* Formt

* Fix copy dashboards

* Fixes onUpdate onCopy handlers

* Pylint

* Update tests

* Clean up

* Handle data on show

* Change Save to Apply

* Update Cypress save test

* Update Cypress edit prop test

* Update PropertiesModal test

* Fix duplicate request with cross filters

* Improve code style

* Fix typo

* Lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants